Make `Rc<T>::deref` and `Arc<T>::deref` zero-cost #132553

EFanZh · 2024-11-03T07:48:34Z

Currently, Rc<T> and Arc<T> store pointers to RcInner<T> and ArcInner<T>. This PR changes the pointers so that they point to T directly instead.

This is based on the assumption that we access the T value more frequently than accessing reference counts. With this change, accessing the data can be done without offsetting pointers from RcInner<T> and ArcInner<T> to their contained data. This change might also enables some possibly useful future optimizations, such as:

Convert &[Rc<T>] into &[&T] within O(1) time.
Convert &[Rc<T>] into Vec<&T> utilizing memcpy.
Convert &Option<Rc<T>> into Option<&T> without branching.
Make Rc<T> and Arc<T> FFI compatible types where T: Sized.

rustbot · 2024-11-03T07:48:42Z

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

marmeladema · 2024-11-03T09:22:09Z

Would it potentially enable those types to have an ffi compatible ABI? So that they could be returned and passed directly from /to ffi function, like Box?

EFanZh · 2024-11-03T10:36:08Z

Would it potentially enable those types to have an ffi compatible ABI? So that they could be returned and passed directly from /to ffi function, like Box?

I think in theory it is possible, at least for sized types, but I am not familiar with how to formally make it so.

jhpratt · 2024-11-03T23:07:24Z

r? libs

joboet · 2024-11-07T16:17:52Z

@EFanZh Is this ready for review? If so, please un-draft the PR.

EFanZh · 2024-11-07T17:12:22Z

@joboet: The source code part is mostly done, but I haven’t finished updating LLDB and CDB pretty printers. The CI doesn’t seem to run those tests.

joboet · 2024-11-08T12:56:42Z

No worries! I just didn't want to keep you waiting in case you had forgotten to change the state.
@rustbot author

bors · 2025-02-23T16:45:50Z

⌛ Trying commit ae9240c with merge 1a76f3d...

Make `Rc<T>::deref` and `Arc<T>::deref` zero-cost Currently, `Rc<T>` and `Arc<T>` store pointers to `RcInner<T>` and `ArcInner<T>`. This PR changes the pointers so that they point to `T` directly instead. This is based on the assumption that we access the `T` value more frequently than accessing reference counts. With this change, accessing the data can be done without offsetting pointers from `RcInner<T>` and `ArcInner<T>` to their contained data. This change might also enables some possibly useful future optimizations, such as: - Convert `&[Rc<T>]` into `&[&T]` within O(1) time. - Convert `&[Rc<T>]` into `Vec<&T>` utilizing `memcpy`. - Convert `&Option<Rc<T>>` into `Option<&T>` without branching. - Make `Rc<T>` and `Arc<T>` FFI compatible types where `T: Sized`.

bors · 2025-02-23T18:44:17Z

☀️ Try build successful - checks-actions
Build commit: 1a76f3d (1a76f3df0b6373e760df2514a5af2587f3e01aff)

rust-timer · 2025-02-23T19:59:33Z

Finished benchmarking commit (1a76f3d): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.2%, 0.5%]	5
Regressions ❌ (secondary)	0.4%	[0.3%, 0.8%]	8
Improvements ✅ (primary)	-0.6%	[-1.5%, -0.2%]	8
Improvements ✅ (secondary)	-1.4%	[-2.6%, -0.4%]	5
All ❌✅ (primary)	-0.2%	[-1.5%, 0.5%]	13

Max RSS (memory usage)

Results (primary 2.0%, secondary -0.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	5.3%	[1.6%, 13.9%]	6
Regressions ❌ (secondary)	1.9%	[1.9%, 1.9%]	1
Improvements ✅ (primary)	-3.0%	[-5.2%, -1.7%]	4
Improvements ✅ (secondary)	-3.6%	[-3.6%, -3.6%]	1
All ❌✅ (primary)	2.0%	[-5.2%, 13.9%]	10

Cycles

Results (primary -1.2%, secondary -1.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.2%	[-1.4%, -1.0%]	2
Improvements ✅ (secondary)	-1.9%	[-2.4%, -1.5%]	2
All ❌✅ (primary)	-1.2%	[-1.4%, -1.0%]	2

Binary size

Results (primary 0.2%, secondary -0.9%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.0%, 1.6%]	54
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-1.5%, -0.0%]	24
Improvements ✅ (secondary)	-0.9%	[-6.9%, -0.1%]	46
All ❌✅ (primary)	0.2%	[-1.5%, 1.6%]	78

Bootstrap: 772.144s -> 774.372s (0.29%)
Artifact size: 359.81 MiB -> 359.75 MiB (-0.02%)

library/alloc/src/raw_rc.rs

EFanZh · 2025-02-26T01:03:12Z

@rustbot ready

bors · 2025-03-07T17:17:09Z

☔ The latest upstream changes (presumably #138155) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2025-03-08T15:58:55Z

☔ The latest upstream changes (presumably #138208) made this pull request unmergeable. Please resolve the merge conflicts.

scottmcm · 2025-03-22T19:04:46Z

Neutral-ish on icounts, improved on cycles, and even shrinks optimized binaries? Nice.

scottmcm · 2025-03-22T19:18:39Z

library/alloc/src/raw_rc.rs

+//!
+//! - Making reference-counting pointers have ABI-compatible representation as raw pointers so we
+//!   can use them directly in FFI interfaces.
+//! - Converting `Option<Rc<T>>` to `Option<&T>` with a memory copy operation.


Hmm, this one should optimize to that already with what you've already written here, right?

You could consider adding a codegen test, like

use std::sync::Arc; #[no_mangle] pub fn option_arc_as_deref_is_nop(x: &Option<Arc<i32>>) -> Option<&i32> { // CHECK-LABEL: @option_arc_as_deref_is_nop(ptr // CHECK: %[[R:.+]] = load ptr, ptr %x // CHECK: ret ptr %[[R]] x.as_deref() }

scottmcm · 2025-03-22T19:25:38Z

library/alloc/src/raw_rc.rs

+impl Deref for RcLayout {
+    type Target = Layout;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}


Hmm, if "external" things should only use the inner layout field through this deref, would it be worth putting RcLayout in a separate module to enforce that with privacy?

(This is one of those places that really wants to be able to just do unsafe struct RcLayout(Layout); to enforce it that way...)

scottmcm · 2025-03-22T19:45:26Z

library/alloc/src/raw_rc.rs

+trait RcLayoutExt {
+    /// Computes `RcLayout` at compile time if `Self` is `Sized`.
+    const RC_LAYOUT: RcLayout;
+}


Nice that we only need one of these, since Rc and Arc can shared the constant 👍

scottmcm · 2025-03-22T19:51:03Z

library/alloc/src/raw_rc.rs

+unsafe fn ref_counts_ptr_from_value_ptr(value_ptr: NonNull<()>) -> NonNull<RefCounts> {
+    const REF_COUNTS_OFFSET: usize = size_of::<RefCounts>();
+
+    unsafe { value_ptr.byte_sub(REF_COUNTS_OFFSET) }.cast()
+}


ymmv: since you need to cast in this function anyway, you could consider avoiding the need for the cast-to-unit in the callers of this by having this be something like

Suggested change

unsafe fn ref_counts_ptr_from_value_ptr(value_ptr: NonNull<()>) -> NonNull<RefCounts> {

const REF_COUNTS_OFFSET: usize = size_of::<RefCounts>();

unsafe { value_ptr.byte_sub(REF_COUNTS_OFFSET) }.cast()

}

unsafe fn ref_counts_ptr_from_value_ptr<T: ?Sized>(value_ptr: NonNull<T>) -> NonNull<RefCounts> {

unsafe { value_ptr.cast::<RefCounts>().sub(1) }

}

(That ought to simplify the MIR too, since byte_sub has to cast to NonNull<u8> then cast back again, but if you cast and can just sub you avoid that step. Of course the conversions like that are optimized out by LLVM anyway, but...)

scottmcm · 2025-03-22T19:56:08Z

library/alloc/src/raw_rc.rs

+/// Get a pointer to the strong counter object in the same allocation with a value pointed to by
+/// `value_ptr`.
+///
+/// # Safety
+///
+/// - `value_ptr` must point to a value object (can be uninitialized or dropped) that lives in a
+///   reference-counted allocation.
+unsafe fn strong_count_ptr_from_value_ptr(value_ptr: NonNull<()>) -> NonNull<UnsafeCell<usize>> {
+    const STRONG_OFFSET: usize = size_of::<RefCounts>() - mem::offset_of!(RefCounts, strong);
+
+    unsafe { value_ptr.byte_sub(STRONG_OFFSET) }.cast()
+}
+
+/// Get a pointer to the weak counter object in the same allocation with a value pointed to by
+/// `value_ptr`.
+///
+/// # Safety
+///
+/// - `value_ptr` must point to a value object (can be uninitialized or dropped) that lives in a
+///   reference-counted allocation.
+unsafe fn weak_count_ptr_from_value_ptr(value_ptr: NonNull<()>) -> NonNull<UnsafeCell<usize>> {
+    const WEAK_OFFSET: usize = size_of::<RefCounts>() - mem::offset_of!(RefCounts, weak);
+
+    unsafe { value_ptr.byte_sub(WEAK_OFFSET) }.cast()
+}


suggestion: Can you avoid doing manual layout calculations here?

If you converted to a NonNull<RefCounts> first, then &raw can just mention the field to get its pointer, rather than needing to offset_of and deal in raw bytes.

(I think that'd let you drop the repr(C) on RefCounts too, which would be nice. I don't think there should be a need for it -- I don't think any of the logic here really cares whether the strong or weak count is first in memory.)

rustbot assigned jhpratt Nov 3, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 3, 2024

EFanZh force-pushed the zero-cost-rc-arc-deref branch from b283c44 to ae36f44 Compare November 3, 2024 09:14